fix(permissions): handle None response from ACP request_permission#13457
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the ACP → Hermes permission-approval bridge by safely handling an unexpected None result from request_permission, preventing attribute errors and defaulting to a safe deny.
Changes:
- Return
"deny"whenrequest_permissionresolves toNonein the approval callback. - Add a unit test covering the
Noneresponse case to ensure the callback denies safely.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
acp_adapter/permissions.py |
Adds an explicit guard for None permission responses, defaulting to deny. |
tests/acp/test_permissions.py |
Adds regression coverage to ensure None responses produce "deny". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.warning("Permission request timed out or failed: %s", exc) | ||
| return "deny" | ||
|
|
||
| if response is None: |
There was a problem hiding this comment.
response is None is handled by returning "deny", but it fails silently. If None represents an unexpected/invalid ACP reply, consider logging (at least debug/warning) or adding a short comment explaining why None is an expected value, so diagnosing permission-bridge issues is easier in production.
| if response is None: | |
| if response is None: | |
| logger.warning( | |
| "Permission request returned no response; denying by default " | |
| "(session_id=%s, command=%r)", | |
| session_id, | |
| command, | |
| ) |
| if response is None: | ||
| return "deny" |
There was a problem hiding this comment.
PR description still contains template placeholders (e.g., Fixes #, empty Changes Made / How to Test). Please fill these in so reviewers and release notes can trace the bug report and verify the reproduction + fix steps.
What does this PR do?
This PR hardens the ACP ? Hermes permission-approval bridge by safely handling an unexpected None result from
equest_permission, preventing attribute errors and defaulting to a safe deny.
Related Issue
Fixes #13449
Type of Change
Changes Made
equest_permission resolves to None in the approval callback.
How to Test
Checklist
Code
Documentation & Housekeeping